[MIR-borrowck] Two phase borrows #46537

pnkfelix · 2017-12-06T12:05:44Z

This adds limited support for two-phase borrows as described in
http://smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/

The support is off by default; you opt into it via the flag -Z two-phase-borrows

I have written "limited support" above because there are simple variants of the simple v.push(v.len()) example that one would think should work but currently do not, such as the one documented in the test compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs

(To be clear, that test is not describing something that is unsound. It is just providing an explicit example of a limitation in the implementation given in this PR. I have ideas on how to fix, but I want to land the work that is in this PR first, so that I can stop repeatedly rebasing this branch.)

rust-highfive · 2017-12-06T12:05:48Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

nikomatsakis · 2017-12-06T14:03:41Z

src/test/run-pass/borrowck/two-phase-baseline.rs

+
+fn main() {
+    let mut v = vec![0, 1, 2];
+    v.push(v.len());


nikomatsakis · 2017-12-06T14:21:07Z

src/librustc_mir/dataflow/drop_flag_effects.rs

-    lookup_result: LookupResult,
-    each_child: F)
-    where F: FnMut(MovePathIndex)
+pub(crate) fn place_needs_drop<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,


Nit: formatting is funky. I'm moving these days towards rustfmt-style:

pub(crate) fn place_needs_drop( tcx: TyCTxt<...> ..., ) -> bool { ... }

nikomatsakis

I left various documentation nits.

I'd personally prefer to tweak the "use closures for everything" change to dataflow as well; either to keep the tcx or to thread through the required state as a generic that implements a trait with the method definitions, rather than various closures. I found closures a bit confusing because (a) they move the code out from the fn body into its caller, but (imo) it sort of logically belongs in the callee; (b) they suggest that caller can alter this behavior at will, but I'm not sure that's true; (c) they tend to play poorly with the borrow checker anyway vs a trait, since the borrow checker won't allow two closures to share a mutable borrow (since it doesn't know they don't reach one another).

Anyway, there is one test I didn't see. We want to make sure that people don't start a shared borrow during the reservation period that then continues to be used after the mutable borrow has been activated. Did you have a test for this that I missed? (I actually don't know if the code will handle this correctly or not. I think maybe not, because I believe it requires some kind of special check in the borrow checker at activation time.)

fn read(_: &i32) { } 

fn ok() {
    let mut x = 3;
    let y = &mut x;
    { let z = &x; read(z); }
    *y += 1;
}

fn not_ok() {
    let mut x = 3;
    let y = &mut x;
    let z = &x;
    *y += 1;
    read(z); //~ ERROR somewhere in here
}

nikomatsakis · 2017-12-06T14:21:50Z

src/librustc_mir/dataflow/drop_flag_effects.rs

+    erased_ty.needs_drop(gcx, param_env)
+}
+
+pub(crate) fn on_lookup_result_bits<'tcx, F, U>(move_data: &MoveData<'tcx>,


I'd love to see a comment. e.g. what the heck is has_uniform_drop_state supposed to mean?

nikomatsakis · 2017-12-06T14:22:01Z

src/librustc_mir/dataflow/drop_flag_effects.rs

-    move_path_index: MovePathIndex,
-    mut each_child: F)
-    where F: FnMut(MovePathIndex)
+pub(crate) fn on_all_children_bits<'tcx, F, U>(move_data: &MoveData<'tcx>,


Nit: formatting

nikomatsakis · 2017-12-06T14:24:17Z

src/librustc_mir/dataflow/drop_flag_effects.rs

+
+        // let gcx = tcx.global_tcx();
+        // let erased_ty = gcx.lift(&tcx.erase_regions(&ty)).unwrap();
+        // if erased_ty.needs_drop(gcx, ctxt.param_env) {


Nit: Dead code.

nikomatsakis · 2017-12-06T14:25:45Z

src/librustc_mir/dataflow/impls/mod.rs

            location,
-            |path, s| Self::update_bits(sets, path, s)
-        )
+            |lv| place_contents_drop_state_cannot_differ(self.tcx, self.mir, lv),


My concern with this is that there is more-or-less only one "right thing" you can do here in this closure, right? Or is there ever a time that we want other behavior?

If indeed there all closures are always the same, why not make on_lookup_result_bits abstract over some type T: OnLookupResultBitHelpers, which is only implemented by TyCtxt or something?

One advantage to this approach is that its trivial to add instrumentation to the individual closures. Also, you need both the tcx and the mir, so to get the full effect here (in terms of avoiding having to thread either of those down) I'd have to at the very least make a struct that held both of them, rather than just passing either on its own.

Having said that, I was considering moving to a trait instead of arbitrary closures, largely because I wanted a single thing to carry both of the closures that I occasionally pass in. So I'll look into that.

(Oh, maybe you mean make it a method on tcx that takes the mir as an argument, and then pass in a closure that does |arg| self.tcx.new_method(self.mir, arg) ...? Not sure how that's better/clearer that |arg| current_func(self.tcx, self.mir, arg), though...)

nikomatsakis · 2017-12-06T14:30:10Z

src/librustc_mir/dataflow/mod.rs

+    ///
+    /// When its false, no local clone is constucted; instead a
+    /// mutable reference directly into `on_entry` is passed along via
+    /// `SetTriple.on_entry` instead.


Is the intention that, if this returns false, the on_entry bit set will not be modified by statement_effect and terminator_effect (but merely used as immutable input)? If so, let's say so explicitly.

Oh, yes, that is indeed my intent. Will fix.

(The fact that a mutable reference is passed is just an artifact of how support for the true case is implemented.)

nikomatsakis · 2017-12-06T15:20:52Z

src/librustc_mir/dataflow/impls/borrows.rs

+    }
+}
+
+struct FindPlaceUses<'a, 'b: 'a, 'tcx: 'a> {


I feel like the purpose of this struct should be documented here.

Also, I think at this point this file is getting quite complex. We might consider breaking out submodules.

(I addressed the doc request but chose not to fragment the file into submodules; I want to wait until after the impl period to do the latter, for the sake of everyone rebasing...)

nikomatsakis · 2017-12-06T15:21:50Z

src/librustc_mir/dataflow/impls/borrows.rs

+            PlaceContext::Validate => false,
+
+            // pure overwrites of an lvalue do not activate it. (note
+            // LvalueContext::Call is solely about dest lvalue)


Nit: s/LvalueContext/PlaceContext/

nikomatsakis · 2017-12-06T15:22:10Z

src/librustc_mir/dataflow/impls/borrows.rs

+                        // (This can certainly happen in valid code. I
+                        // just want to know about it in the short
+                        // term.)
+                        info!("encountered use of Place {:?} of borrow_idx {:?} \


Nit: debug! ?

oops. will fix.

nikomatsakis · 2017-12-06T15:24:09Z

src/librustc_mir/dataflow/impls/borrows.rs

-    fn terminator_effect(&self,
-                         sets: &mut BlockSets<BorrowIndex>,
-                         location: Location) {
+    fn statement_effect_on_activations(&self,


It's mildly surprising that this function also applies the statement effect on reservations. Perhaps rename to statement_effect_on_activations_and_reservations?

(Alternatively move that call into the caller, but I think it's maybe nice to have them grouped together here? I guess I could go either way, just want the name to be clear on what is affected.)

yeah I considered having that call in the caller, but I thought at some point there might be a case where the activation effect would want to observe state from both before and after the reservation effect. (This turned out not to be the case in the code so far, but I didn't want to build in an assumption that things would stay that way.)

Anyway the idea in my head is more that the ActiveBorrows bitvector holds both activations and reservations state. So maybe the problem is the naming here, I need some way to succinctly say "the combined reservation and activation state."

PhasedBorrows maybe? And statement_effect_on_phased_borrows ?

I think I'm going to change this so that there's a single method and we pass a is_activations: bool parameter representing the context.

nikomatsakis · 2017-12-06T15:28:50Z

src/librustc_mir/borrow_check/mod.rs

-        'next_borrow: for i in flow_state.borrows.elems_incoming() {
-            // FIXME for now, just skip the activation state.
-            if i.is_activation() { continue 'next_borrow; }
+        let mut elems_incoming = flow_state.borrows.elems_incoming();


Pre-existing, but I feel like the enclosing function here deserves a comment explaining what it does, and I would expect this (quite clever, actually) bit here to be in that comment (i.e., invokes with either a RESERVATION or ACTIVATION for each borrowed path, whichever is more specific).

(addressed by new commit injected earlier in commit history)

pnkfelix · 2017-12-06T15:51:53Z

Anyway, there is one test I didn't see. [...]

Ack, you are right! Even after you've pointed out several times that the activation point needs this check, I thought what I had for the reservation point would suffice.

Will fix!

nikomatsakis · 2017-12-06T15:56:07Z

Update: this test seems to compile for me:

#![allow(dead_code)]

fn read(_: &i32) { } 

fn ok() {
    let mut x = 3;
    let y = &mut x;
    { let z = &x; read(z); }
    *y += 1;
}

fn not_ok() {
    let mut x = 3;
    let y = &mut x;
    let z = &x;
    *y += 1;
    read(z); //~ ERROR somewhere in here
}

Example:

> rustc --stage1 ~/tmp/pnkfelix.rs -Zborrowck=mir -Ztwo-phase-borrows
> // look ma, no errors

bors · 2017-12-06T21:09:18Z

☔ The latest upstream changes (presumably #46268) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2017-12-07T16:52:43Z

@nikomatsakis okay, pushed commit that adds support for checking the activation points explicitly against other borrows that overlap it, and added your test (with some extra variants added to show ... a current oddity in the implementation...)

Going to look into tweaking the "use closures for everything" change to dataflow next, before addressing other nits (and rebasing to resolve the upstream conflicts).

pnkfelix · 2017-12-08T10:57:11Z

@nikomatsakis what do you think of the name trait PlaceObservations for a trait that gathers together fn needs_drop and fn has_uniform_drop_state?

pnkfelix · 2017-12-08T10:57:24Z

(and is solely implemented by (tcx, mir) ...)

carols10cents · 2017-12-12T18:54:00Z

r? @nikomatsakis

this also doubles as a rereview/question answering ping for you!

@nikomatsakis what do you think of the name trait PlaceObservations for a trait that gathers together fn needs_drop and fn has_uniform_drop_state?

Made `do_dataflow` and related API `pub(crate)`.

Having the HIR local id is useful for cases like understanding the ReScope identifiers, which are now derived from the HIR local id, and thus one can map an ReScope back to the HIR node, once one knows what those local ids are.

pnkfelix · 2017-12-13T15:19:46Z

Okay I think I have incorporated all review feedback (and gone through yet another semi-nasty rebase....)

Lets get this landed! 😄

…taflow. This is meant to ease development of multi-stage dataflow analyses where the output from one analysis is used to initialize the state for the next; in such a context, you cannot start with `bottom_value` for all the bits.

… intrablock state. (Still musing about whether it could make sense to revise the design here to make these constraints on usage explicit.)

…ace`.

…d IdxSet.

arielb1 · 2017-12-13T20:01:15Z

src/librustc_mir/borrow_check/mod.rs

@@ -504,9 +529,9 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
                let data = domain.borrows();
                flow_state.borrows.with_elems_outgoing(|borrows| {
                    for i in borrows {
-                        let borrow = &data[i];
+                        // FIXME: does this need to care about reserved/active distinction?


no because it doesn't care about the borrow being immutable/mutable.

arielb1 · 2017-12-13T20:01:49Z

src/librustc_mir/borrow_check/error_reporting.rs

+            // a terminator location.
+            return None;
+        }
+
        let local = if let StatementKind::Assign(Place::Local(local), _) =


Can you just switch this to be a .get?

match self.mir[location.block].statements.get(location.statement_index) { Statement { kind: StatementKind::Assign(...), ..) => { // ..

arielb1 · 2017-12-13T20:07:50Z

src/librustc_mir/borrow_check/mod.rs

+            if self.reservation_error_reported.contains(&place_span.0) {
+                debug!("skipping access_place for activation of invalid reservation \
+                        place: {:?} borrow_index: {:?}", place_span.0, borrow_index);
+                return AccessErrorsReported { mutability_error: false, conflict_error: false };


conflict_error should return true because an error had already been reported (earlier).

arielb1 · 2017-12-13T20:11:55Z

src/librustc_mir/borrow_check/mod.rs

-                (Read(_), BorrowKind::Shared) => Control::Continue,
+                // Obviously an activation is compatible with its own reservation;
+                // so don't check if they interfere.
+                (Activation(_, activating), _) if index.is_reservation() &&


what's the idea of this? If there's a reservation going around a loop, this could actually cause unsoundness.

First off: Good catch. I definitely need to think more about this.

Explanation: The reason for this check here is to ensure that the activation is not treated as a conflict with its own reservation. (Any time we reach a activating use, it is by definition going to have a reservation for that borrow also in the current flow state -- because if there wasn't a reservation in the flow state, then we would not consider the use an activation.)

But your loop example is important, and may show a big flaw in how I've approached this.

I think I've made a mistake in that the current Reservations dataflow is tracking what borrows may reach a location, but for the scenario I'm trying to catch here, it might be necessary to use a must-reach semantics. Building up that flow analysis and employing it (at least for the desired filter here) might be a way to resolve the loop example you give (because with must-reach, a control-flow merge on entry to the loop will remove the reservations that are introduced within the loop itself).

Having said that, such an analysis is a pretty big addition to the framework here. I need to think more on this.

(Another potential answer, suggested by @nikomatsakis, might be to treat the reservation in a loop as an error.)

arielb1 · 2017-12-13T20:19:51Z

src/librustc_mir/borrow_check/mod.rs

+                        Reservation(_) => {
+                            debug!("recording invalid reservation of \
+                                    place: {:?}", place_span.0);
+                            this.reservation_error_reported.insert(place_span.0.clone());


so this error suppression might actually fix #45697 if it's moved out of this loop.

arielb1 · 2017-12-13T22:15:46Z

r=me mod. comments

…minators. (Same net effect as code from before; just cleaner way to get there.)

…of_scope_at_location. Instead we are "just" careful to invoke it (which sets up a bunch of kill bits) before we go into the code that sets up the gen bits. That way, when the gen bits are set up, they will override any previously set kill-bits for those reservations or activations.

…ror reported at *any* point in past.

…th activations.

…efore activation" In reality the currently generated MIR has at least one of the activations in a copy that occurs before the merge. But still, good to have a test, in anticipation of that potentially changing...

Instead, filter out (non-)conflicts of activiations with themselves in the same manner that we filter out non-conflict between an activation and its reservation.

pnkfelix · 2017-12-14T23:49:57Z

Okay I think I've addressed almost all of the comments above.

There are some extant bugs that I'll need to address (I only now attempted turning the flag off by default and seeing what happens with all the current -Z borrowck=mir test cases), but I think I want to land this first and then deal with those afterwards.

Huzzah, all the "bugs" identified in the compile-fail tests are not bugs at all, they are showing a feature where -Z borrowck=mir -Z two-phase-borrows behaves somewhat like non-lexical lifetimes even without passing -Z nll

(the five identifies failures are discussed in the details block below)

test	errors	evaluation
compile-fail/borrowck/borrowck-describe-lvalue.rs	22 expected errors not found	test is written assuming lexical lifetimes + single-phase borrows (i.e. just mutably borrows with no use to introduce conflict); will fix
compile-fail/borrowck/borrowck-match-already-borrowed.rs	compiled successfully!	(ibid)
compile-fail/borrowck/borrowck-overloaded-index-ref-index.rs	2 expected errors not found	(ibid)
compile-fail/borrowck/borrowck-union-borrow.rs	6 expected errors not found	(ibid)
compile-fail/coerce-overloaded-autoderef.rs	1 unexpected error found, 1 expected error not found	lol this is just the same error being reworded, perhaps due to a change to traversal order? (i.e. ~~"mut also borrowed as imm"~~ ⇒ "imm also borrowed as mut")

pnkfelix · 2017-12-15T00:21:34Z

@bors r+

bors · 2017-12-15T00:21:35Z

📌 Commit 159037e has been approved by pnkfelix

arielb1 · 2017-12-15T00:23:08Z

@bors r-

arielb1 · 2017-12-15T00:23:30Z

That should have been an r=arielb1 or r=nikomatsakis

arielb1 · 2017-12-15T03:21:09Z

@bors r+

bors · 2017-12-15T03:21:10Z

📌 Commit 159037e has been approved by arielb1

arielb1 · 2017-12-15T03:22:39Z

However, it's probably a wise idea to limit reservations to assignments to temporaries. I don't think MIR construction ever does anything else, but you shouldn't be able to do this:

let mut a = 0;
let mut b = 0;
let mut x: &mut u32 = &mut a;
let y = &mut x;
*y = &mut b; // this borrow should be active
// look at me, no use of `*y`, but:
use(x, &b); // should be illegal

bors · 2017-12-15T05:40:19Z

⌛ Testing commit 159037e with merge 84feab3...

[MIR-borrowck] Two phase borrows This adds limited support for two-phase borrows as described in http://smallcultfollowing.com/babysteps/blog/2017/03/01/nested-method-calls-via-two-phase-borrowing/ The support is off by default; you opt into it via the flag `-Z two-phase-borrows` I have written "*limited* support" above because there are simple variants of the simple `v.push(v.len())` example that one would think should work but currently do not, such as the one documented in the test compile-fail/borrowck/two-phase-reservation-sharing-interference-2.rs (To be clear, that test is not describing something that is unsound. It is just providing an explicit example of a limitation in the implementation given in this PR. I have ideas on how to fix, but I want to land the work that is in this PR first, so that I can stop repeatedly rebasing this branch.)

bors · 2017-12-15T08:13:27Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 84feab3 to master...

rust-highfive assigned arielb1 Dec 6, 2017

nikomatsakis reviewed Dec 6, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2017

nikomatsakis requested changes Dec 6, 2017

View reviewed changes

rust-highfive assigned nikomatsakis and unassigned arielb1 Dec 12, 2017

nikomatsakis mentioned this pull request Dec 12, 2017

MIR Borrowck: two-phased borrows #46037

Closed

pnkfelix added 2 commits December 13, 2017 00:15

Revised graphviz rendering API to avoid requiring borrowed state.

93c4ffe

Made `do_dataflow` and related API `pub(crate)`.

pnkfelix force-pushed the two-phase-borrows branch from 0302cfd to 9451942 Compare December 13, 2017 14:38

pnkfelix added 5 commits December 13, 2017 13:50

Refactoring: Allow BlockSets.on_entry to denote locally accumulated…

e123117

… intrablock state. (Still musing about whether it could make sense to revise the design here to make these constraints on usage explicit.)

Refactoring alpha-rename place (BorrowData field) to `borrowed_pl…

39e126c

…ace`.

Implement Borrow/BorrowMut/ToOwned relationships betweed IdxSetBuf an…

e437e49

…d IdxSet.

Allow mir::Place to be used as a key in hashtables.

ef64ace

arielb1 reviewed Dec 13, 2017

View reviewed changes

pnkfelix added 6 commits December 13, 2017 18:07

Address review comment: use .get instead of indexing to cope w/ ter…

3c7d9ff

…minators. (Same net effect as code from before; just cleaner way to get there.)

Address review note: AccessErrorsReported meant to track whether er…

b75248e

…ror reported at *any* point in past.

Address review feedback: don't bother skipping reservations paired wi…

b0421fa

…th activations.

Address review feedback: don't treat "first" activation special.

159037e

Instead, filter out (non-)conflicts of activiations with themselves in the same manner that we filter out non-conflict between an activation and its reservation.

pnkfelix mentioned this pull request Dec 15, 2017

add #![feature(nll)] to supplement -Znll #46716

Closed

pnkfelix added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 15, 2017

pnkfelix added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 15, 2017

bors merged commit 159037e into rust-lang:master Dec 15, 2017

bors mentioned this pull request Dec 15, 2017

nll part 5 #46733

Merged

pnkfelix mentioned this pull request Dec 15, 2017

two phase borrows: ensure reservations are limited to assignments to temps #46746

Closed

bluss mentioned this pull request Dec 21, 2017

Slower matrixmultiply in nightly-2017-12-16 #46923

Closed

est31 mentioned this pull request Dec 29, 2017

Performance regression with nightly benchmarks. #47062

Closed

pnkfelix mentioned this pull request Feb 22, 2018

NLL: Redo two-phase borrows with conservative, narrow focus #48431

Closed

pnkfelix mentioned this pull request Mar 28, 2019

Tracking issue for mutable_borrow_reservation_conflict compatibility lint #59159

Closed

3 tasks

[MIR-borrowck] Two phase borrows #46537

[MIR-borrowck] Two phase borrows #46537

Conversation

pnkfelix commented Dec 6, 2017

rust-highfive commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Dec 6, 2017

nikomatsakis commented Dec 6, 2017

bors commented Dec 6, 2017

pnkfelix commented Dec 7, 2017 • edited Loading

pnkfelix commented Dec 8, 2017

pnkfelix commented Dec 8, 2017

carols10cents commented Dec 12, 2017

pnkfelix commented Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielb1 Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

arielb1 commented Dec 13, 2017

pnkfelix commented Dec 14, 2017 • edited Loading

pnkfelix commented Dec 15, 2017

bors commented Dec 15, 2017

arielb1 commented Dec 15, 2017

arielb1 commented Dec 15, 2017

arielb1 commented Dec 15, 2017

bors commented Dec 15, 2017

arielb1 commented Dec 15, 2017 • edited Loading

bors commented Dec 15, 2017

bors commented Dec 15, 2017

pnkfelix Dec 6, 2017 •

edited

Loading

pnkfelix commented Dec 7, 2017 •

edited

Loading

pnkfelix commented Dec 13, 2017 •

edited

Loading

arielb1 Dec 13, 2017 •

edited

Loading

pnkfelix commented Dec 14, 2017 •

edited

Loading

arielb1 commented Dec 15, 2017 •

edited

Loading